feat: execution recovery safeguards - #97

First PR here - CONTRIBUTING.md covers the conventions, and ISSUES.md covers when a change needs an accepted issue first.

What this changes

12 new files, +175/-0. docs/execution-recovery-v1/contract.md states six normative rules R1-R6 and a table mapping each of 10 JSON fixtures to the rule it exercises; the fixtures land under testdata/execution_recovery_v1/; README_EXECUTION_RECOVERY.md is a root-level pointer to both. No Go code, no test, no CI change - go build ./cmd/kh produces the same binary, and go test ./... never walks testdata/.

Does it match the description

Undersells. The summary names the fixtures and the rules but not README_EXECUTION_RECOVERY.md, a new root-level file in a repo whose root holds exactly one README and which nothing links to. The Test plan's three items are all unchecked, which is an accurate account of the gap below.

Blocking

• testdata/execution_recovery_v1/completed_with_tx.json:3-4 - the envelope matches no type in this repo, and Go will not tell you. The fixtures nest under "execution" with "id"; cmd/execute/status.go:16-26 is flat with executionId and no wrapper. json.Unmarshal(fixture, &ExecStatusResponse{}) therefore returns a nil error and a fully zero struct - Status == "" for all seven execution-shaped fixtures. A table test keyed on Status silently takes the empty-string branch for every one of them and still passes. -> record the fixtures in the flat wire shape with httpStatus as a sidecar, or add the envelope type and loader in this PR.

• testdata/execution_recovery_v1/reverted.json:5,13 - the fixture for the failure this pack exists to prevent escapes every rule in it. It is "status": "completed" with "verified": true and "receiptStatus": "reverted". R2 (contract.md:21) fires only when "no transaction hash / verified receipt is present" - one is present and verified. R4 (:29) lists reverted as a status, and this fixture's status is completed. No rule reads receiptStatus, so a client implementing R1-R6 literally returns success for a reverted on-chain transaction. contract.md:48 maps this fixture to R4, which cannot see it. The Definitions line at :10 says "verified successful receipt", but Definitions are not phrased as operative rules. -> promote receiptStatus == "success" into R2's rule text.

• docs/execution-recovery-v1/contract.md:25,29,37 - three of the six MUSTs are contradicted by the client shipped in this repo, and nothing in the document says so. R3 requires a stable idempotency key across write retries: internal/http/client.go:50-60 sets RetryMax = 3 and excludes only 429, so a 502 on kh ex transfer replays the buffered POST up to three times, and no idempotency key exists anywhere in the repo - up to four transfers for one user intent, which is the single most dangerous case in the pack and the one with no fixture at all. R6 says clients SHOULD ride out a first-read not_found: cmd/execute/transfer.go:195-196 returns an API error on any non-200, so --wait aborts on the cold-start 404 the pack ships a fixture for. R4 declares reverted and not_found terminal; transfer.go:30-33 recognises only completed and failed, and cmd/run/status.go uses a third vocabulary (success/error/cancelled) that appears in no rule. -> add a conformance column to the fixture table, or mark each unimplemented rule as normative-but-not-yet-met.

Mechanical - actionable as-is

• testdata/execution_recovery_v1/cold_start.json:2,5 - httpStatus: 200 with "status": "queued" is decision-identical to queued.json, so it exercises R1, not R6. R6 is about a first read that returns not_found; only not_found.json can exercise it, and contract.md:49 already maps it there. Make this a two-response sequence (404 then 200) or drop it.
• testdata/execution_recovery_v1/malformed.json - valid JSON, so it cannot exercise R4's "unparseable/malformed bodies" clause. It unmarshals cleanly to Status == "", which appears in no rule's status list, so a client polls a dead execution forever. Split R4 into terminal-failure statuses and unrecognised-schema, and ship a genuinely non-JSON file if that path is meant to be covered.
• README_EXECUTION_RECOVERY.md:1 - carries a UTF-8 BOM (ef bb bf before the #). No other file in the repo has one, and contract.md correctly does not.
• docs/execution-recovery-v1/contract.md - published nowhere, so the audience it names at :4 cannot read it. .github/workflows/sync-cli-docs.yml:59-60 copies only kh*.md and :73 hand-lists quickstart.md concepts.md. It also breaks nothing: docs/generate.go:47 globs kh*.md, so docs-check stays green.

Needs a decision

• Should the pack land without a consumer? - (a) merge as fixtures plus prose, costs nothing now, but go test ./... never walks testdata/ so the fixtures and the contract can drift apart indefinitely with green CI; (b) require the loader and table test in this PR, costs more work now and is what would have caught the two blocking items above and both fixture items.
• Where should a hand-written normative document live? - (a) docs/, which is generated from the cobra tree with two hand-written exceptions enumerated by name in the sync workflow, so a third needs adding to that list; (b) outside docs/, stated as repo-internal, costing the doc its published home.

Verdict

Changes requested - the fixtures decode to an empty struct against this repo's own type, and the one fixture representing a reverted transaction is classified as success by the rules shipped beside it.

Thanks for the thorough review — addressed in 3505b09.

Wire shape + consumer (blocking)

• Fixtures are now the flat direct-execution shape (executionId, not nested execution.id) with httpStatus as a sidecar.
• Added internal/execrecovery loader + table tests so go test ./internal/execrecovery/... actually consumes every fixture. Empty-struct silent pass is no longer possible.

Reverted receipts (blocking)

• R2 now treats receiptStatus=reverted as Failure even when status=completed and verified=true.
• --wait fails closed on reverted receipts (cmd/execute/transfer.go + tests).

R3 idempotency

• kh ex transfer / kh ex cc set Idempotency-Key once before Do, so go-retryablehttp retries reuse the same key.
• --idempotency-key pins a stable key across process restarts.
• Covered by TestTransferCmd_IdempotencyKeyStableAcrossHTTPRetries.

R6 cold start

• --wait / --watch tolerate an initial HTTP 404/not_found until the wait deadline.
• cold_start.sequence.json is a real 404 → pending → completed+success sequence (old cold_start.json removed).

Malformed / vocabulary / docs

• malformed.json uses a genuine non-JSON responseRaw.
• Direct-execution vs workflow-run vocabularies documented and tested separately.
• Removed BOM root README_EXECUTION_RECOVERY.md; published hand-written docs/execution-recovery.md and registered it in sync-cli-docs.yml.

Note on CI check-issue-link

Title now references #53. That check still requires the accepted label on the issue — we cannot apply that label ourselves. Happy to wait on maintainer acceptance of #53 (or no-issue-required if preferred).

Happy to adjust further if anything still misses the mark.

What this changes

Net diff against current main: 26 files, +1036/-40. internal/execrecovery is a new package (classify.go, fixture.go, idempotency.go, vocabulary.go + two test files) implementing and testing the R1-R6 execution-recovery contract from docs/execution-recovery-v1/contract.md. cmd/execute/transfer.go and cmd/execute/status.go gain cold-start-tolerant polling and reverted-receipt failure handling; transfer.go and contract_call.go gain a --idempotency-key flag wired to set Idempotency-Key once before client.Do, so go-retryablehttp's automatic retries reuse it. Ten JSON fixtures plus one multi-step sequence fixture live under testdata/execution_recovery_v1/. docs/execution-recovery.md is new and is now wired into the doc-sync pipeline (docs/generate.go, .github/workflows/sync-cli-docs.yml, +1/-1) so it actually reaches docs.keeperhub.com instead of sitting unpublished.

I built and ran the full suite locally against pull/97/head (Go 1.25, matching go.mod): go build ./... and go vet ./... are clean, go test ./... is green across every package, including all of this PR's new tests.

Does it match the description

Close, with two small gaps. The Test Plan checklist in the PR body still shows three unchecked boxes ("port/load fixtures in Go tests," "assert decision table," "confirm fixture IDs synthetic") - all three are now actually done; the checkboxes are just stale. Separately, the summary doesn't mention the --idempotency-key flag added to two commands, the cold-start-polling behavior change, or the one-line sync-cli-docs.yml addition that makes the doc publishable - all real changes worth a line in the description, even though none of them are risky.

Blocking

None. Every blocking item from the previous review is resolved and independently re-verified against the current head:

• The flat-vs-nested envelope mismatch that silently zero-decoded seven fixtures is fixed - fixtures now use executionId/status flat shape, and TestFixtures_DecodeIntoDirectStatus fails the build if any 200-status fixture decodes to an empty Status.
• reverted.json is no longer classified as success - classify.go's hasRevertedReceipt check runs unconditionally, before the success path, regardless of RequireChainEvidence. Covered by TestRevertedIsNeverSuccess and, end to end, TestTransferCmd_WaitFailsOnRevertedReceipt (which I ran against a real httptest server).
• R3 (idempotency), R4 (terminal/malformed vocab), and R6 (cold-start 404 tolerance) are no longer contradicted by the shipped client - the CLI's own transfer.go/status.go/contract_call.go now implement all three, each with a passing test.

Mechanical - actionable as-is

• internal/execrecovery/fixture.go's Fixture struct tags are misaligned (extra padding before string/bool/etc.) - gofmt -l flags it. Trivial: gofmt -w internal/execrecovery/fixture.go. (Most of the repo isn't gofmt-clean either, so this is cosmetic, not a blocker - but it's this PR's own new file.)
• contract_call.go's idempotency wiring is a byte-for-byte mirror of transfer.go's, but only transfer.go has a test proving the key survives an HTTP retry (TestTransferCmd_IdempotencyKeyStableAcrossHTTPRetries). Given the mechanism is identical, this is low risk, but a one-line equivalent test for contract_call.go would close the gap cheaply.
• Update the PR description's Test Plan checkboxes to reflect that all three items are done.

With the team

#53 asks for public fixtures covering the workflow-webhook submission response (POST /api/workflows/<id>/webhook - the envelope third-party adapter authors parse after submitting a signed receipt). This pack fixtures a different, real endpoint: GET /api/execute/{id}/status, the CLI's own direct-execution polling surface. That's now confirmed at the code level too - vocabulary.go explicitly separates DirectExecutionVocabulary() (/api/execute/{id}/status) from WorkflowRunVocabulary() (/api/workflows/executions/{id}/status), and a test (TestVocabularySurfacesAreDistinct) enforces the two never share a terminal-status string. Neither vocabulary is the webhook-submission envelope #53 describes.

This pack is genuinely good, tested, now-published work on its own terms - a real gap (the CLI's polling/retry/reverted-receipt behavior had no fixture coverage or tests at all) closed thoroughly. It just isn't what #53 asked for. I'm taking the question of whether #53 should stay open with this PR referenced as related-but-not-resolving, or whether a new issue should be filed specifically for the webhook-envelope fixtures, to the team - I'd lean toward the latter, since this PR shouldn't be blocked or rescoped to also cover the webhook envelope, a materially different response shape with a different consumer (external adapter authors, not the CLI itself). I'll follow up on #53 once that's settled.

Verdict

Approve. Every prior blocking and mechanical finding is fixed and independently verified (build, vet, and full test suite pass against the actual PR head, not just by reading the diff). The remaining item - the endpoint mismatch with #53 - isn't a defect in this PR; it's a scoping question I'm taking to the team separately, and it shouldn't hold up merging good, tested, standalone work.

What this changes

internal/execrecovery is a new package classifying one status observation into pending / success / failure / malformed / rate_limited, driven by ten JSON fixtures under testdata/execution_recovery_v1/. Alongside it, four production changes: --idempotency-key on kh ex transfer and kh ex cc, which set an Idempotency-Key header on every POST whether or not the flag is passed (transfer.go:85-95, contract_call.go:93-103); 404 tolerance in the --wait poll loop (transfer.go:169-185) and in the --watch loop (status.go:143-147); receipt rows plus a new execOutcomeError that fails a run when a receipt reports reverted (transfer.go:252-266, status.go:125); and a new public guide.

The workflow change is .github/workflows/sync-cli-docs.yml:73, which adds execution-recovery.md to the hand-written-guide list. That workflow runs on workflow_dispatch and release: published, checks out KeeperHub/keeperhub with secrets.KEEPERHUB_PAT, wraps each named guide in Nextra frontmatter into docs/cli/, and opens a PR against staging. So that one line makes docs/execution-recovery.md a customer-facing page on docs.keeperhub.com at the next release. It grants no new permissions and touches no gate.

Since the last review the diff is one commit, d6e72e2, touching two files.

Previously raised

• gofmt on internal/execrecovery/fixture.go: addressed, struct tags aligned at :11-20.
• A contract-call test mirroring the transfer retry test: addressed, cmd/execute/contract_call_test.go:308-364.
• Stale Test Plan checkboxes: addressed.
• Description missing --idempotency-key, the cold-start change, and the sync-cli-docs.yml line: addressed, all three are in the body.

Four corrections on my side, because they bear on how far my earlier reviews here can be trusted:

• You were right about #53, and I was wrong to leave it hanging over this PR as an open scoping question. Every structural claim in your 2026-08-13 comment checks out: the webhook handler, its schema and its integration test all live in KeeperHub/keeperhub, the success envelope is 200 {executionId, status:"running"}, there is no fixtures repo, this CLI has no call path to the webhook, and #53's proposed 201/422/503 fixture names match none of the codes that route returns. Those fixtures belong next to the handler. I am removing decision-needed.
• My first review's blocker - that the CLI recognises only completed and failed while R4 declares reverted and not_found terminal - was wrong in the other direction. Those are receipt-level and transport-level values, not execution statuses; the CLI's two-status set matches the server enum. The contract's vocabulary needed correcting, not the client.
• My second review granted "R3, R4 and R6 are no longer contradicted by the shipped client" without checking the handler. R3 is half-implemented and R6 overshoots in --watch, both below.
• The approve on this PR was mine and I gave it too early.

The idempotency header itself is the right call and I am not asking for it to come out - before it, the same 502 replay produced a second real execution. Only the response it unlocks is unhandled.

Does it match the description

Undersells. The body names every file, but test: is the wrong type for a diff adding two user-facing flags and changing the control flow of two fund-moving commands. release-please-config.json carries no changelog-sections override, so test: gets no changelog entry and no version bump, and --idempotency-key would ship invisible. One bullet also claims more than the code does: "fail closed without chain evidence" is listed as implemented, but nothing in the CLI ever sets RequireChainEvidence, so classify.go:124-133 is reachable only from fixtures.

Split test: not one unit. The fixture pack plus internal/execrecovery is correct with everything else reverted; the idempotency header is correct with the 404 tolerance reverted; the 404 tolerance is correct with the receipt handling reverted; the docs page plus the sync line is correct with all the Go changes reverted. Four shippable sides.

Blocking

• cmd/execute/transfer.go:104, cmd/execute/contract_call.go:113 - sending the header unconditionally makes a 409 reachable that the CLI could never receive before, and both sites classify on the status code alone -> lib/idempotency.ts on the app repo's staging returns 409 for both idempotency_conflict (retryable:false) and idempotency_in_progress (retryable:true), separated only by code. internal/http/client.go sets RetryMax=3, so a 504 during a slow on-chain wait replays the POST, the replay carries the same key, hits the in-flight lock and returns 409; transfer.go:104 surfaces it as an error, the user re-runs, ResolveIdempotencyKey("") mints a fresh key, and the transfer broadcasts twice. Fix: branch on the body's code at both sites - retry the same key on idempotency_in_progress, report a payload mismatch on idempotency_conflict - or gate the header behind an explicit --idempotency-key until that handling lands.

• cmd/execute/status.go:143-147 - the 404 continue runs inside a loop with no deadline. watchExecStatus on main terminates only on a terminal status or an error, and this converts the one error that reliably terminated it into a retry -> kh ex st <id> --watch against a mistyped id exits immediately today; at head it spins forever, and under --json or a non-TTY it spins silently with no output. This is reachable with a well-formed id: app/api/execute/[executionId]/status/route.ts filters on organizationId, so another org's execution also answers 404. Fix: give --watch a deadline and scope 404-as-pending to it, or leave watchExecStatus alone and keep the tolerance in pollExecStatus, which already has one at transfer.go:159.

• docs/execution-recovery.md:16 and docs/execution-recovery-v1/contract.md:33 - the rule this pack is built around describes a response the API cannot produce, and sync-cli-docs.yml:73 publishes it -> completeExecution in app/api/execute/_lib/execution-service.ts re-verifies every claimed hash before writing completed; allVerified is every(r => r.verified) in lib/web3/verify-receipt.ts, a reverted receipt is written verified:false, and that file's header states a hash that cannot be positively confirmed "resolves to verified: false, never verified: true". Reverted is conclusive, so the row settles as failed. {status:"completed", verified:true, receiptStatus:"reverted"} is precisely the state the KEEP-966 gate exists to make unreachable, yet it is what reverted.json presents as a wire sample and what the guide tells integrators to expect. contract.md:18 compounds it by listing queued as a direct-execution status when the enum in app/api/execute/_lib/types.ts is pending|running|unconfirmed|completed|failed. Fix: restate the receipt rule as a client-side invariant ("do not infer success from status alone"), drop queued, mark reverted.json as a defensive fixture rather than an observed envelope - or drop the sync-cli-docs.yml line from this PR and publish once the wording is checked against the handler.

Mechanical - actionable as-is

• internal/execrecovery/classify.go:141, cmd/execute/transfer.go:261 - only one of four non-success receipt states is treated as failure. lib/db/schema-extensions.ts defines success | reverted | not_found | timeout | safe_inner_failure; a safe_inner_failure receipt returns nil from execOutcomeError, so --wait exits 0 on a Safe whose inner call failed. Match receiptStatus != "success", or enumerate all five.
• internal/execrecovery/classify.go:102-110 - the switch accepts queued, not_found, error, cancelled and success, none of which the endpoint emits, and three of which are the workflow vocabulary the doc comment at :60-62 forbids feeding here while TestVocabularySurfacesAreDistinct asserts the two never overlap. Meanwhile default: returns malformed, so a status the server adds later reads as a corrupt body.
• cmd/execute/transfer.go:179 - statusResp.Status == "not_found" is unreachable; the endpoint answers 404 with {"error":"Execution not found"} and no status field.
• cmd/execute/transfer.go:219 - dead branch; the StatusNotFound check returns exactly what the != StatusOK check on the next line already returns.
• internal/execrecovery/fixture_test.go:63 - TestFixtures_ClassifyTable has no len(fixtures) guard, so it passes on an empty load. Renaming any fixture to *.sequence.json makes fixture.go:50-55 skip it silently and the table still goes green. Add the guard used at :27, or assert an expected count.
• internal/execrecovery/fixture.go:11-20 - nothing carries or checks a version. The directory says _v1 but Fixture has no version field and json.Unmarshal ignores unknown keys, so a v2 fixture dropped here is graded by v1 rules silently. Require a version and fail the load when it is not 1.
• Fixture fidelity against the real envelope: code:"not_found" in not_found.json and cold_start.sequence.json is never sent; verifiedAt is required on DirectExecutionReceiptEntry and every fixture omits it; chainId is optional server-side and every fixture sets it; and queued.json is named for a status its body does not contain ("status":"pending"), so the queued branch has no coverage.
• internal/execrecovery/classify.go:135 and contract.md:34 reference --require-verified, which does not exist in this repo.
• classify.go:31,39 duplicate ExecStatusResponse/ExecReceipt from status.go:18-43 field for field, which is why the exported HasRevertedReceipt at classify.go:158 cannot be called from cmd/execute and execOutcomeError re-implements it. One definition should go.
• Retitle to feat: so the two new flags reach the changelog.

With the team

• Does the receipt-classification layer earn its place now that the server gate makes completed plus a reverted receipt unreachable? Keeping it is fail-closed defence in depth and costs a second copy of the wire types plus a rule set that drifts from the handler with no CI signal in this repo; dropping it relies on status, which execOutcomeError already reads correctly, and loses the guard if the gate regresses. I lean toward keeping it and relabelling it a client-side invariant, but I want the API owners on it. I am discussing this with the core team now and will come back with a verdict shortly.
• Should --wait exit non-zero when it times out on unconfirmed? The reconciler is still watching by design, and the server's own comment warns that calling such a write failed is what invites the retry that broadcasts a second transaction. Exiting non-zero keeps scripts honest about not having a confirmed result but risks that double send; exiting zero with an explicit unconfirmed status is safer for funds and weaker as a gate. Unchanged by this PR, but this pack is the first thing to name unconfirmed as first-class. I am discussing this with the core team now and will come back with a verdict shortly.

Verdict

Changes requested, on three items introduced by the production half of this diff: an unhandled 409 on the write path, an unbounded --watch loop, and a customer-facing docs page stating behaviour the handler contradicts. The fixture pack and loader are not what I am holding, and this increment closed both items I left open.